Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use monotonic time for process state tracking #468

Closed
wants to merge 1 commit into from

Conversation

mook-as
Copy link

@mook-as mook-as commented Jul 21, 2014

This makes sure system time changes (e.g. via ntp) doesn't result in odd assertion failures due to processes entering the running state before being started.

Lightly tested on Linux; the BSD code is untested (written based on man clock_gettime and FreeBSD headers). Unfortunately I do not know how to fix it for OSX, and suspect that falling back to time.time() is a bad idea.

While running tox locally seems to pass, I do not know how much to trust it.

Fixes #281.

This makes sure system time changes (e.g. via ntp) doesn't result in odd
assertion failures due to processes entering the running state before being
started.
mook-as pushed a commit to hpcloud/supervisor that referenced this pull request Jul 22, 2014
This makes sure system time changes (e.g. via ntp) doesn't result in odd
assertion failures due to processes entering the running state before being
started.

Supervisor#281
Supervisor#468
https://bugs.activestate.com/show_bug.cgi?id=104508
https://activestate.atlassian.net/browse/STO-859
@gjcarneiro
Copy link

Disclaimer: I'm not a supervisor developer, just trying to do a review.

Looks good except the part where you use ctypes. Personally I would rather have supervisor try to import time.monotonic(), if available, or else fall back to using time.time() as usual:

import time
monotonic_time = getattr(time, "monotonic", time.time)

If this means that the monotonic issue is fixed only on Python 3, so be it. Python 3 has been widely available for many years, it's time to stop backporting every single feature to Python 2.

@mook-as
Copy link
Author

mook-as commented Oct 28, 2014

Hi! Thanks for looking; anything's better than radio silence for three months 😉

The reason I used ctypes was because (for whatever silly reason) supervisord's running on Python 2 on Linux in my deployment; I was simply scratching my own itch in this case. While in general I agree that Python 3 is the future, I still need to support existing software.

Having said that, getting even just the Python 3 part in (assuming somebody who can merge it in makes that call - it sounds like you aren't that person?) is better than nothing, since it would at least make patching in Python 2 support easier. But it feels like amending the PR at this point without knowing that would have a better chance of landing isn't productive right now.

@gjcarneiro
Copy link

Hi. I am in the same boat, have a patch that needs review, sitting there for some weeks. So I thought that the supervisor maintainers must need some help, so I am reviewing some patches to help the project.

Yes, I think if only the Python 3 patch is accepted, falling back to Python 2 time.time(), it keeps supervisord running as before on Python 2, but improves the behaviour if running on Python 3. Nonetheless, perhaps you can monkey-patch the time module somewhere: import time; time.monotonic = my_fancy_monotonic_function, before the supervisor process.py module gets imported.

@mnaberez
Copy link
Member

@mook-as Thanks for your work on this. I know the original motivation for this was #281, which I have hopefully fixed in eb96a2a. It would be great if you were able to help test that the fix works.

I have been thinking about this PR, and I'm a bit nervous about the ctypes hacking that's needed to get this on Python 2. As far as I can tell, Python 2 is a very large majority of our users and probably will be for some time, and they are all going to hit this code path. It seems risky. I'd be a lot more comfortable if we were able to just defer to Python stdlib for this.

The suggestion from @gjcarneiro is only use monotonic time if it is available. How about something like this in compat.py:

try:
    from time import monotonic as maybe_monotonic_time
except ImportError:
    from time import time as maybe_monotonic_time

The maybe_monotonic_time name is ugly but it would make calling code clear that time might go backwards. On Python 2, #281 would be fixed by eb96a2a. On Python 3, monotonic would be used so eb96a2a would be a no-op. I'm not crazy about differing behavior across Python versions but it would add stability so it might be worth it.

@mook-as @gjcarneiro What do you think?

@mnaberez
Copy link
Member

cc @msabramo @lukeweber You are guys are pretty active lately, what do you think about this PR?

@mnaberez
Copy link
Member

mnaberez commented Oct 7, 2015

Closing for now since the motivation for this was #281 which should be fixed by eb96a2a. We can reopen if discussion starts again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Crash at system time change back
3 participants